Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed Keeweb application for NC 28.x.x #231

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Fixed Keeweb application for NC 28.x.x #231

merged 2 commits into from
Jan 4, 2024

Conversation

florian-forestier
Copy link
Contributor

@florian-forestier florian-forestier commented Jan 3, 2024

Hi,

This MR aim to fix #229 and #230.

The most important part of this MR is the viewer.js file. As Nextcloud removed the OCA.Files API, I had to use the new one. The original problem (CORS issue) was solved by adding $csp->addAllowedScriptDomain("'unsafe-inline'"); to lib/Controller/PageController.php. Tested on Nextcloud 28.0.1.

I also seen #221. Please note that I am interested in continuing your work on this Nextcloud plugin, as I use it on a daily basis, for personal and professional use.

I also also take the opportunity of this PR to thank you for the job you've done over the years on this project, having access to my keepass files directly on my Nextcloud saved me several times 😄

Note: don't be affraid of the +6,515 added lines, 99.9% of them are in the package-lock.json file...

👋

@arnowelzel
Copy link
Collaborator

Thanks for your help, great!

Did you check, if that version still works with NC 27 as well? If not, we need to increase the minimum supported version to 28 as well and keep the older version for NC 27 and below unchanged.

@florian-forestier
Copy link
Contributor Author

Hi,

It partially works for NC 27 (file association is broken due to changes in viewer.js). It would be better to increase the minimum supported version.

@arnowelzel
Copy link
Collaborator

Hi,

It partially works for NC 27 (file association is broken due to changes in viewer.js). It would be better to increase the minimum supported version.

Ok, thanks for the feedback. I'll change the supported version since NC 27 is still maintained and the older version works fine there.

Since this version will not work fully with Nextcloud 27, it is better to  only publish it for Nextcloud 28 and higher.
@arnowelzel arnowelzel merged commit 7ac3915 into jhass:master Jan 4, 2024
@arnowelzel
Copy link
Collaborator

arnowelzel commented Jan 4, 2024

Hmm - it does not work here with NC 28 either :-(

Clicking a KDBX file will open Keeweb - but the file is not opened then.

Edit: the URL called then is https://myserver.example/apps/keeweb/?open=/test.kdbx - which is correct, since test.kdbx is in the main folder. But Keeweb does not open it - it will just present the start page without the file.

@florian-forestier
Copy link
Contributor Author

Looking on it, I didn't had this problem on my previous test

@arnowelzel
Copy link
Collaborator

May it helps to know, that my server responds with HTTP 404 to the request, eventhough the file exists:

image

image

@florian-forestier
Copy link
Contributor Author

Okay, it seems there is two issues:

  • First of all, Keeweb could not load some content because of CSP (again). There is a missing addAllowedScriptDomain("blob:");. I don't know why this error appears only now.
  • Secondly, it seems that Nextcloud servers which are not configured to enforce HTTPS (without overwriteprotocol in config.php) experience some issues loading files. As HTTPS is required to make Keeweb works, the simple (but working) solution is to change all HTTP calls to HTTPS in the PageController.php file.

The following patch should solve these issues:

diff --git a/keeweb/lib/Controller/PageController.php b/keeweb/lib/Controller/PageController.php
index 005ced5..847afac 100644
--- a/keeweb/lib/Controller/PageController.php
+++ b/keeweb/lib/Controller/PageController.php
@@ -87,6 +87,7 @@ class PageController extends Controller {
 	public function config($file) {
 		$csrfToken = \OC::$server->getCSRFTokenManager()->getToken()->getEncryptedValue();
 		$webdavBase = \OCP\Util::linkToRemote('webdav');
+        $webdavBase = str_replace("http://", "https://", $webdavBase);
 		$config = ['settings' => (object) null];
 		if (isset($file)) {
 			$config['files'] = [
@@ -136,6 +137,7 @@ class PageController extends Controller {
 		$csp->addAllowedConnectDomain("'self'");
 		$csp->addAllowedConnectDomain('https://services.keeweb.info');
 		$csp->addAllowedScriptDomain('https://plugins.keeweb.info');
+        $csp->addAllowedScriptDomain("blob:");
 		$csp->addAllowedScriptDomain("'unsafe-inline'");
 		$csp->addAllowedConnectDomain('https://plugins.keeweb.info');
         $csp->allowEvalScript(true);

Tested on a fresh NC28 install. @arnowelzel can you try on your side and confirm ?

@arnowelzel
Copy link
Collaborator

Sorry - still does not work here. Same effect: KeeWeb is loaded but does not open the file. Server is using HTTPS (as it was before).

Same error in the console:

GET https://myserver.example/apps/keeweb/?open=/test.kdbx 404 (Not Found)

About this line - I don't think we need that. When the instance is not using HTTPS the resource can not be loaded anyway, since it will not be available using HTTPS:

$webdavBase = str_replace("http://", "https://", $webdavBase);

@florian-forestier
Copy link
Contributor Author

I can't reproduce from my side on a fresh and a old Nextcloud server, using Firefox 115.6.0esr (normal & private browsing), and Chromium 120.0.6099.129 (normal & private browsing)... Also checked using Chrome and Brave on mobile (last version available on Play Store) without issue.

My server is deployed using the official docker image, with a Traefik acting as reverse-proxy. Can you give more information on how your Nextcloud is deployed, so I could create the same environment to check ?

@arnowelzel
Copy link
Collaborator

I don't use Docker but a plain server setup with NC 28 using Apache 2.4, PHP-FPM 8.3 and MariaDB.

To be sure I repeated the test with a fresh setup - and now it works. The replacement of http with https is not needed since KeeWeb would not work when loaded via HTTP anyway.

I will apply the change $csp->addAllowedScriptDomain("blob:"); and then prepare a new release.

@arnowelzel
Copy link
Collaborator

Now the release action does not work any longer :-(.

bin/release: line 65: hub: command not found
Error: Process completed with exit code 127.

@jhass Can you have a look what is missing? I can't update the workflow actions, just trigger them.

@jhass
Copy link
Owner

jhass commented Jan 4, 2024

Github deprecated hub in favor of gh a while ago, I guess they removed the former from the GHA runner images now. I fixed the release script to use gh over hub.

Thanks guys!

@arnowelzel
Copy link
Collaborator

Thanks for the quick response and help!

@bkraul
Copy link

bkraul commented Jan 6, 2024

@florian-forestier is da real MVP. Thank you for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal server error
4 participants